fix(contracts): enforce CEI in withdraw and cancel_stream to close reentrancy surface#947
Conversation
|
heads up: main's ci was broken (the Backend CI and Backend Docker Image CI jobs) until the fixes in #969 and #974 just landed, so the red backend/docker checks on this pr are almost certainly stale, they ran against the broken main. please rebase to re-test against the now-green main: |
…e reentrancy surface (LabsCrypt#789) Both `withdraw` and `cancel_stream` previously executed token transfers before persisting stream state, violating Checks-Effects-Interactions. A token contract with a transfer hook could re-enter either function while storage still held the pre-update state, enabling a double payout. Changes: - Rename `transfer_and_update_stream` → `apply_withdrawal`, which now follows CEI: update stream fields → save_stream (effects committed) → token transfer. - `withdraw` delegates to `apply_withdrawal`; the separate `save_stream` call is removed since persistence is now handled inside the helper. - `cancel_stream` is restructured so all state mutations and `save_stream` happen before either token transfer (recipient payout + sender refund). - Two regression tests added: one for `withdraw` and one for `cancel_stream`, each asserting that a second call at the same timestamp fails because committed state already reflects the first operation. Closes LabsCrypt#789 Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
patopatrish
force-pushed
the
fix/789-cei-reentrancy-withdraw-cancel
branch
from
0222915 to
ae7204b
Compare
ogazboiz
left a comment
ogazboiz
left a comment
There was a problem hiding this comment.
correct CEI refactor. apply_withdrawal now does the effects (withdrawn_amount +=, status/active updates) then save_stream then the token transfer, so state is committed before the external call, closing the reentrancy/double-payout window, and the redundant second save_stream in withdraw is gone. cancel does the same: updates withdrawn_amount + refunded_amount (derived from the updated withdrawn) + status, saves, then both transfers, so the invariant recipient+sender == deposited holds and the two regression tests prove it. cargo test green (99). merging.
2 participants
Closes #789
Problem
withdrawandcancel_streamboth transferred tokens before committing updated stream state to storage, violating the Checks-Effects-Interactions (CEI) pattern.token_addressis a caller-supplied contract. A token with a transfer hook could re-enterwithdraworcancel_streamwhile persistent storage still reflected the pre-update state (stalewithdrawn_amount,is_active = true), enabling a double payout.Affected code paths
transfer_and_update_stream(called bywithdraw)token_client.transferfired before updatingwithdrawn_amount/is_active, andsave_streamran even later, after the helper returnedcancel_streamtoken_client.transfercalls fired beforesave_streamFix
apply_withdrawal(replacestransfer_and_update_stream) — new helper that enforces CEI:withdrawn_amount, setlast_update_time, flipis_active/statusif fully drainedsave_stream— state is now durable before any external calltoken_client.transferwithdraw— delegates toapply_withdrawal; the standalonesave_streamcall is removed (now inside the helper).cancel_stream— restructured so all state mutations +save_streamprecede both token transfers (recipient payout and sender refund).Regression tests added
test_withdraw_state_committed_before_transfer_prevents_double_payout— after a successful withdraw, a second call at the same ledger timestamp (simulating a re-entrant hook) returnsInvalidAmountbecause committed state already reflects the withdrawal.test_cancel_state_committed_before_transfers_prevents_double_cancel— aftercancel_stream, a second cancel attempt returnsStreamInactivebecause the stream was marked inactive before either transfer fired.Files changed
contracts/stream_contract/src/lib.rscontracts/stream_contract/src/test.rs